-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Relocate tests in tests/ui
#140029
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Relocate tests in tests/ui
#140029
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! So, the new locations and names for these tests look good -- however, I'd like to see some short doc comments on the test intentions (they were not very obvious to me).
Please also squash the commits into one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion: I honestly have no idea what this test is actually trying to exercise. This goes all the way back to 2010 d6b7c96.
I assume this is trying to exercise some short-circuiting behavior of ||
and &&
but the test itself makes it hard to tell.
EDIT: the OG version of this Rust was
fn incr(mutable &int x) -> bool {
x += 1;
check (false);
ret false;
}
fn main() {
auto x = (1 == 2) || (3 == 3);
check (x);
let int y = 10;
log x || incr(y);
check (y == 10);
if (true && x) {
check (true);
} else {
check (false);
}
}
so yeah, I believe this is indeed a ||
and &&
short-circuiting test. It has a no-arg closure, presumably because back then there must've been a case where the parser confused ||
and the no-arg closure form || EXPR
or sth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's amusing is that this is one of the listings of the archive of OG rust syntax listed at https://brson.github.io/archaea/lazy-and-or.html/.
EDIT: I realized you can actually see the dynamic evolution of this test on that listing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the analysis and sharing that interesting link! Now I understand that this test verifies short-circuiting behavior of logical operators.
I'll update the test with clearer naming and comments to better express its purpose while preserving the original functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And thanks for the historical context! That's super helpful 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion: so this one is also introduced in the 2010 populate tree commit, and it kinda has been preserved all these years modulo some language changes. I believe this is some kind of "positive" smoke test of a tree of owned memory...?
Originally (like back in Rust 0.8), this was
tag list {
cons(int,@list);
nil;
}
fn main() {
cons(10, @cons(11, @cons(12, @nil)));
}
This is also one of the OG rust syntax examples at https://brson.github.io/archaea/list.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remark: yeah, this test is because unary negation on an owned string didn't use to properly typeck, back in de4b383.
I'm rolling a second compiler reviewer for a second opinion on "what on earth are these tests actually testing". r? jieyouxu |
This comment was marked as off-topic.
This comment was marked as off-topic.
21f48ba
to
13c22da
Compare
This comment has been minimized.
This comment has been minimized.
13c22da
to
48528d8
Compare
Part of #133895
Moved tests from a top-level directory into a more appropriate subdirectory.
If there is anything else that could be improved, please let me know!
r? jieyouxu